-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plugin/kubernetes: Add useragent #6484
base: master
Are you sure you want to change the base?
Conversation
23337ca
to
bd0c7b7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6484 +/- ##
==========================================
+ Coverage 55.70% 59.22% +3.52%
==========================================
Files 224 252 +28
Lines 10016 13473 +3457
==========================================
+ Hits 5579 7979 +2400
- Misses 3978 4902 +924
- Partials 459 592 +133 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, minor nit about formatting.
Also, what about including runtime.Version()
so we know what compiler version was used?
Sure we can add it, not sure if the compiled golang version provides any additional value here? |
Usually when trying to identify a build, I like to have as much unique info as possible. I also usually like to include the git branch name. Looks like we don't have this in the current coremain package. |
Have the comments been addressed? Not sure since commits were squashed. |
I haven't addressed the comments yet. |
What's an example of how it could be useful? |
I don't have one (and I would favor not exposing it). @SuperQ any thoughts why you would need this? |
@SuperQ @chrisohaver anything else I need to do before this is ready to be merged? |
Having the compiler version can be useful in finding bugs/security issues. |
Wouldn't you rather rely on container scanning or similar for that? I think the purpose of a useragent is to help identifying the client and not to provide extended metadata (because right now it says kubernetes, which is misleading). |
Scanners are solving a different issue. This is about having the context inline at the time of use. |
In kubernetes' audit logs you'll see: "userAgent":"coredns/v0.0.0 (linux/amd64) kubernetes/$Format" This change adds a userAgent to the requests made by CoreDNS against the kubernetes API: "userAgent":"CoreDNS/v1.11.1 git_commit:ae2bbc29be1aaae0b3ded5d188968a6c97bb3144 (linux/amd64/go1.22)" Signed-off-by: Manuel Rüger <manuel@rueg.eu>
I've included the runtime.Version(), I personally still lack the benefit but eventually rather have a few bytes more in my log storage system than having trying to guess which coredns version logged it. :) This is what it will look like now:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
1. Why is this pull request needed and what does it do?
CoreDNS doesn't set a UserAgent and client-go will fallback to the default one https://github.com/kubernetes/client-go/blob/master/rest/config.go#L498
In kubernetes' audit logs you'll see:
This change adds a userAgent to the requests made by CoreDNS against the kubernetes API:
2. Which issues (if any) are related?
3. Which documentation changes (if any) need to be made?
4. Does this introduce a backward incompatible change or deprecation?